[Draft] Fix DP userspace crashes and memory-allocation failures for compressed playback on PTL - #11181
Conversation
|
Most seem valid fixes. The IPC empty list remove was just merged to main #11157 |
| unsigned int i; | ||
|
|
||
| if (!ctx || !ctx->n_mod) | ||
| return -ENOENT; |
There was a problem hiding this comment.
all three of these changes are needed? With the below 2 changes, this one might not be needed any more?
There was a problem hiding this comment.
only the ctx->n_mod check is needed
| * The pointers it contains reference the IPC mailbox, which stays | ||
| * valid and is covered by the DP thread's SOF_DP_PART_CFG partition. | ||
| */ | ||
| struct module_ext_init_data ext_data; |
There was a problem hiding this comment.
members here are called after IPCs where they are used, so maybe init_instance would be a better name for this one
| /* | ||
| * SOF_IPC4_MOD_INIT_INSTANCE: a by-value copy of the caller's | ||
| * ext_data. The original lives on the calling thread's stack, | ||
| * which the DP thread's memory domain doesn't grant access to. |
| * accessible memory; the original caller-stack copy is out of reach here. | ||
| */ | ||
| pmod->priv.cfg.ext_data = &flat->ext_data; | ||
| flat->ret = ops->init(pmod); |
There was a problem hiding this comment.
ah, that's because the only tested until now module in DP mode was SRC and it doesn't access .ext_data in its struct module_interface instance
| /* Allocate buffer memory for module. sof_ctx_alloc() routes through the | ||
| * module's vregion when one is configured (e.g. DP/userspace modules), | ||
| * falling back to sof_heap_alloc()/rballoc() otherwise - same as | ||
| * mod_alloc_ext(). Calling sof_heap_alloc() directly here would bypass |
There was a problem hiding this comment.
AI comments are often way too detailed. You don't need to describe all wrong possibility, so no need to mention why sof_heap_alloc() is wrong here
|
|
||
| #if CONFIG_SOF_USERSPACE_APPLICATION | ||
| if (dev->task) | ||
| if (dev->task && dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP) |
There was a problem hiding this comment.
only some very special LL scheduled components have their dev->task pointers set. IIRC only those running on a different core, which we shouldn't have. Could you check where this .task comes from?
There was a problem hiding this comment.
Let me quote the detective I have hired for this:
Confirmed — dev->task for an LL component is only ever allocated in idc_prepare() (src/idc/idc.c:186-189), explicitly guarded by "we're running LL on different core, so allocate our own task". So a non-NULL dev->task on an LL component is a direct signal that component is being IPC-controlled cross-core via IDC, exactly matching what you described.
This does happen in this topology [1]: pipe1 has an LL task explicitly scheduled on core 1 (ll_schedule.zephyr_ll_task_schedule_common: ... [core 1]), separate from the IPC/pipeline-connect logic on core 0 — a genuine cross-core LL component, not a constructed edge case. That's the component that was incorrectly routed into the DP-only teardown path before this fix.
[1] sof-sdca-amp-compr.tplg, sof-sdca-jack-compr.tplg
There was a problem hiding this comment.
@ujfalusi if that is indeed the case, I very much doubt that any of those topologies currently work. As far as I am concerned, cross-core LL linking was hardly ever intensively tested and has long been mostly forgotten and IMHO should rather never be used - if possible. If you manage to get any of those pipelines running and confirm at run-time that one of the components runs on a different core than the rest of the pipeline - we can take this change. But if it's already broken, I'm not sure fixing one location helps it at all, if anything it creates a (false) impression that it should work.
| * (e.g. a neighbour component's list still references it). Matches | ||
| * the vregion_put() in comp_buffer_free(). | ||
| */ | ||
| if (alloc && alloc->vreg) |
There was a problem hiding this comment.
no need to check alloc->vreg, NULL is allowed.
| * the vregion_put() in comp_buffer_free(). | ||
| */ | ||
| if (alloc && alloc->vreg) | ||
| vregion_get(alloc->vreg); |
There was a problem hiding this comment.
I think this isn't quite correct. With this ipc4_comp_connect() would refcount the buffer twice. Can you check on which path this one is called? I don't see any path leading here, where refcount isn't taken yet.
There was a problem hiding this comment.
right, agreed, dropping this patch
| * so it must be coherent - plain cached memory left writes from the | ||
| * creating core invisible to other cores without an explicit flush. | ||
| */ | ||
| vr = rmalloc(SOF_MEM_FLAG_KERNEL | SOF_MEM_FLAG_COHERENT, sizeof(*vr)); |
There was a problem hiding this comment.
until now all accesses to vregion metadata were taking place on the same core. Do you have a DP module on a core different, than the rest of the pipeline?
There was a problem hiding this comment.
it looks like we do — the decoder/encoder consistently ends up on core 1 while the rest of the pipeline (and the IPC thread connecting buffers to it) runs on core 0.
There was a problem hiding this comment.
hm, @jsarha was saying that it didn't work for him IIRC?
| # allocates from dp->mod->priv.resources.alloc), so it must cover the | ||
| # codec's own memory tables plus every connected buffer, not just the codec. | ||
| heap_bytes_requirement 131072 | ||
| heap_bytes_requirement 196608 |
There was a problem hiding this comment.
should be squashed with an earlier one
ae006ba to
6031e32
Compare
|
Changes since v1 (addressing review feedback):
Net: 11 commits → 8. All four codecs (MP3, AAC, FLAC, PCM) re-verified working on hardware after these changes. |
6031e32 to
73c24d7
Compare
The generic widget-common.conf defaults (24576 bytes heap, 8192 bytes stack) are sized for the general case, but Cadence codecs running as DP modules under CONFIG_SOF_USERSPACE_APPLICATION need more of both: - The codec's own memory tables (persist/scratch/input/output) can exceed the default heap budget on their own. On top of that, every buffer connecting to a DP module is also carved out of that same module's vregion (ipc4_comp_connect() allocates from dp->mod->priv.resources.alloc), so the requirement has to cover the codec's memory tables plus all of its connected buffers, not just the codec. - Codec init (e.g. xa_aac_dec, xa_mp3_enc) can need more stack than the default when run inside the DP userspace thread. Bump stack_bytes_requirement to 16384 for both decoder and encoder widgets. Bump heap_bytes_requirement to 196608 for the decoder: AAC and MP3 fit within 131072, but FLAC's own persist/scratch/input/output tables plus its DP-boundary ring buffers measure ~152KB, so 131072 isn't enough once FLAC or PCM are exercised. Encoder is left at 131072, unverified against a codec that needs more. Verified on hardware: AAC, MP3, FLAC and PCM decode all work. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
llext_manager_add_domain()/llext_manager_rm_domain() crashed (llext_manager_mod_find() dereferencing an invalid ctx) when called for a native (non-LLEXT) module scheduled on the DP+userspace-application path. lib_id == 0 is reserved and never populated by a loaded library, but was not guarded against. Skip the LLEXT domain add/remove entirely when LIB_MANAGER_GET_LIB_ID(module_id) == 0. llext_manager_mod_find() also needs its own guard against ctx->n_mod == 0: that's reachable for a genuine (non-NULL) library context, e.g. before its module segments are registered, and without the check the i == 0 case in the loop below reads ctx->mod[-1], out of bounds. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
cfg->ext_data points to a stack variable in module_adapter_new_ext(), valid only synchronously - but SOF_IPC4_MOD_INIT_INSTANCE runs asynchronously in the DP userspace thread, so ops->init() ended up reading a stale pointer into a different thread's stack (privilege violation, not a simple NULL deref). Add ext_data to union scheduler_dp_thread_ipc_param, flatten it by value in ipc_thread_flatten() (copying it while still on the calling thread, before the pointer goes stale), and repoint pmod->priv.cfg.ext_data at the flattened copy in ipc_thread_unflatten_run() before calling ops->init(), clearing it again afterwards to match the existing "not valid outside init()" convention. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
cadence_api_table[] is a compile-time-constant lookup table (codec id -> function pointer), populated once at link time and never written to at runtime, but declared without const - placing it in .data. Zephyr's Xtensa MMU marks .text/.rodata XTENSA_MMU_MAP_SHARED (globally accessible from every memory domain) but not .data, so the DP userspace thread's restricted domain couldn't reach it, causing a privilege-violation crash on the very first codec dispatch. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
z_impl_mod_balloc_align() called sof_heap_alloc(res->alloc->heap, ...) directly, bypassing the module's vregion entirely - unlike mod_alloc_ext(), which correctly checks vreg first via sof_ctx_alloc(). For DP/vregion-scoped modules res->alloc->heap is NULL, so this silently fell through to the global heap/rballoc(), handing back memory outside the DP thread's memory domain. This is what made cd->self (a codec's own library object, allocated via mod_balloc()) unreachable from inside the DP userspace thread. Route through sof_ctx_alloc(res->alloc, ...) like mod_alloc_ext() already does. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
DP components cannot access pipeline objects. Move the End Of Stream flag to the component type. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
struct vregion is created on the DP module's own core (via
vregion_create(), invoked from module_adapter_dp_heap_new()) but is
read and written from other cores too - e.g. buffer_new() on the IPC
core dereferences vr->lifetime.{base,size,used,ptr} for every buffer
connecting to that DP module.
The struct was allocated with plain rmalloc(0, ...), i.e. ordinary
cached memory. Writes made by the creating core can stay in that
core's D-cache indefinitely, invisible to any other core reading the
same physical address, since nothing ever flushes them out. In
practice this showed up as buffer_new() on the IPC core intermittently
seeing an all-zero vregion (base/size/used all 0), so any allocation
failed as "lifetime alloc failed ... free 0" even though the vregion
had over 100KB genuinely free - the metadata core 0 read simply hadn't
propagated from core 1's cache yet.
Allocate the vregion metadata with SOF_MEM_FLAG_KERNEL |
SOF_MEM_FLAG_COHERENT instead, matching the existing pattern used for
other cross-core shared structures in this codebase (e.g. vpage.c,
regions_mm.c). This returns an uncached alias, so every core sees
writes immediately without needing explicit cache maintenance.
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
|
Changes since v2:
|
This series fixes a chain of crashes and allocation failures hit when running compressed-audio decode (Cadence codecs: MP3/AAC/FLAC/PCM) as a DP module under CONFIG_SOF_USERSPACE_APPLICATION. Each fix was isolated and tested individually against hardware; they eliminate every crash/allocation failure seen from pipeline setup through steady-state playback and teardown.
Highlights:
Cross-core cache-coherency bug in vregion_create(): the vregion metadata struct was allocated as plain cached memory even though it's written on the DP module's core and read from other cores (e.g. IPC-core buffer_new()), leading to an intermittent "0 bytes free" allocation failure when the reading core saw stale/zeroed data. Fixed by allocating it as coherent memory — the root cause behind most of the flakiness chased in this series.
Several DP-module-specific bugs: native-module LLEXT domain handling, ext_data not surviving the DP userspace thread boundary, a non-const API table crossing a memory-domain boundary, buffer allocations bypassing the module's vregion, an IPC teardown race, a proc-domain check missing on module free, and a buffer lifetime/vregion refcounting gap.
Topology: bumped the decoder's heap_bytes_requirement (in two steps, ending at 196608 bytes) to cover Cadence's persist/scratch/input/output tables plus the DP-boundary ring buffers for all four codecs — FLAC/PCM need considerably more than MP3/AAC.
Also includes an upstream cherry-pick moving End-of-Stream tracking from the pipeline to the component, since DP components can't access pipeline objects directly.
Verified on hardware: MP3, AAC, FLAC, and PCM compressed playback all work end-to-end (start/pause/reset/teardown) with no crashes.